Skip to content

/ext/standard: Check for empty string in linkinfo()#21793

Merged
Girgias merged 5 commits intophp:masterfrom
LamentXU123:refactor
Apr 20, 2026
Merged

/ext/standard: Check for empty string in linkinfo()#21793
Girgias merged 5 commits intophp:masterfrom
LamentXU123:refactor

Conversation

@LamentXU123
Copy link
Copy Markdown
Contributor

@LamentXU123 LamentXU123 commented Apr 18, 2026

If link_len == 0, it now emits the same warning style as the existing filesystem error path and returns -1 immediately, avoiding the later estrndup() / zend_dirname() work on an empty input as per the TODO message.

@LamentXU123 LamentXU123 requested a review from bukka as a code owner April 18, 2026 01:57
devnexen
devnexen reviewed Apr 18, 2026
View reviewed changes
Comment thread ext/standard/link.c Outdated
@LamentXU123
Copy link
Copy Markdown
Contributor Author

LamentXU123 commented Apr 19, 2026 •
edited
Loading

Ideally we'd be able to audit all the call sites and basically just mandate that the path passed to zend_dirname() must not be empty nor contain null bytes.

AFAIK This can be fixed inside the zend_dirname function. A simple check for the arguments would work (?) We may still need to customize the error message by passing some info (i.e. php function name that use zend_dirname) to the API to make the error message precise.

@Girgias
Copy link
Copy Markdown
Member

Girgias commented Apr 19, 2026

Ideally we'd be able to audit all the call sites and basically just mandate that the path passed to zend_dirname() must not be empty nor contain null bytes.

AFAIK This can be fixed inside the zend_dirname function. A simple check for the arguments would work (?) We may still need to customize the error message by passing some info (i.e. php function name that use zend_dirname) to the API to make the error message precise.

I don't want to pass the argnum to a Zend API, it is better to enforce the semantics at the call site as I can't imagine most use sites of this API have empty strings.

Girgias
Girgias requested changes Apr 19, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UPGRADING also needs to be updated.

Comment thread ext/standard/tests/file/symlink_link_linkinfo_is_link_error1.phpt Outdated
@LamentXU123 LamentXU123 requested a review from Girgias April 20, 2026 01:46
@Girgias Girgias merged commit 3afd6d3 into php:master Apr 20, 2026
19 checks passed
jordikroon added a commit to jordikroon/php-src that referenced this pull request Apr 21, 2026
@jordikroon
Merge branch 'master' into offline-http-tests
394f652 
* master: (26 commits)
  Fix usage of optimize attribute on unsupported compilers (phpGH-21819)
  PHP 8.4 is now for PHP 8.4.22-dev
  ext/phar: remove `phar_archive_data->alias == phar_archive_data->fname` checks (php#21820)
  [skip ci] Mark curl/bug71523.phpt as online test
  [skip ci] Sort paths-ignore and remove cirrus
  [skip ci] Tweak paths-ignore
  ext/standard: Throw a ValueError when the parameter includes NUL bytes in `putenv` and `getenv` (php#21817)
  ext/session: fix missing zval_ptr_dtor for retval in PS_GC_FUNC(user)
  [skip ci] Backport CI changes
  ext/gmp: reject values larger than unsigned long in gmp_pow/binomial/root/rootrem and shift/pow operators.
  Update NEWS for recent bug fixes
  ext/phar: Fix memory leak in phar_verify_signature() when md_ctx is invalid
  phar: propagate phar_stream_flush return value from phar_stream_close
  phar: call phar_entry_delref before goto finish in phar_add_file error paths
  phar: free is_temp_dir entry before rejecting .phar/* paths in offsetGet
  phar: fix NULL dereference in Phar::webPhar() when SCRIPT_NAME is absent
  phar: restore is_link handler in phar_intercept_functions_shutdown
  ext/session: improve parsing of session.cookie_lifetime (php#21704)
  /ext/standard: Check for empty string in linkinfo() (php#21793)
  [Windows] Improve clang-cl support (php#21618)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@devnexen devnexen devnexen left review comments

@Girgias Girgias Girgias approved these changes

@bukka bukka Awaiting requested review from bukka bukka is a code owner

Assignees

No one assigned

Labels

Extension: standard

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@LamentXU123 @Girgias @devnexen